Add type annotations to the module and the unit tests#6
Conversation
|
@ilanbiala: with this, it should be possible to implement full type hinting support for the DBC submodule of cantools... |
|
updated python version tested in CI to 3.10 and 3.14. (python 3.8 has been EOL for a while.) |
andlaus
force-pushed
the
add_type_annotations
branch
from
c3d76d3 to
2ddd392
Compare
|
fun fact: github "simplifies" the |
|
It’s because 3.10 is a number, both in yaml and json. It’s a bit silly that GitHub accepts anything but strings :) |
|
@andlaus is it possible to split out the Python version bump changes into a separate PR and merge that first to isolate this work? |
|
Hey @eerimoq! Sorry to bring this up here, but would it be possible for you to give @andlaus, @zariiii9003, myself, and others more access to the cantools repo and for the readthedocs site for Cantools as well (cantools/cantools#746)? |
yes, but the version bump needs to be done before adding the type hints. be aware that the minimum python version is currently not codified in |
what exactly? |
andlaus
force-pushed
the
add_type_annotations
branch
from
2ddd392 to
a45598c
Compare
|
@eerimoq the Cantools readthedocs site is stuck on a commit that's 2.5 years old and a few version releases out of date (commit revision is at the bottom of the docs site). It looks like permissions haven't been set up correctly for other people to push to the docs site. |
For some of these I am not 100% sure if they are correct. A lot of the code churn of this patch stems from the fact that some classes had to be reordered because python bails out if annotations use classes that are only defined later in the file. This patch features a few minor functional changes which were neccessiated by type annotations: - Support for python2 is no longer declared in `setup.py` - The `Parser.grammar()` methods are supposed to return `Grammar` objects instead of a `Pattern` object which gets implicitly converted to `Grammar` in `Parser.parse()`. IMO this makes the whole affair much less confusing and user code only needs to return `Grammar(x)` instead of `x`. If `grammar()` returns something else, the old behavior is fallen back to, but this will lead to `mypy` complaints in user code... - The `_Token` class is converted from `namedtuple` to a dataclass because named tuples do not play nicely with type annotations and the performance is basically identical. - The internal `_String` class now uses `Pattern` as its base class. (It already had the same API as `Pattern`, but not deriving `_String` from Pattern would necessiate `_String` to be handled separately anyway.) Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com> Approved-by: Christian Hackenbeck <christian.hackenbeck@mercedes-benz.com>
andlaus
force-pushed
the
add_type_annotations
branch
from
a45598c to
79df0af
Compare
|
ok, I rebased this on top of the latest master version, i.e., no need to fiddle with the python versions anymore... |
|
@andlaus could you update the gha config to run mypy in CI? |
|
good idea. I also added a ruff linting step. |
thanks to [at]zariiii9003 for the suggestion! Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
also fix the complaint about the unused import of the `typing` module. Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
andlaus
force-pushed
the
add_type_annotations
branch
from
724b63c to
64b4204
Compare
|
@ilanbiala, @zariiii9003: any objections to merging this? |
| mo = pattern.match(tokens) | ||
|
|
||
| if mo is MISMATCH: | ||
| if isinstance(mo, _Mismatch): |
There was a problem hiding this comment.
I think this is not correct. MISMATCH is a singleton, so here you changed a test of identity to a test of type.
There was a problem hiding this comment.
well, it now matches any instance of the _Mismatch class, not just the MISMATCH singleton. The issue is that type checkers cannot know if _Mismatch objects other than MISMATCH can be returned by pattern.match(), so it will complain if the match object is assumed to be non-_Mismatch after the if mo is MISMATCH test.
There was a problem hiding this comment.
Understood. I wonder if an enum value would work as a placeholder until mypy supports PEP661 sentinel values...
There was a problem hiding this comment.
So this seems to work:
class _Mismatch(Enum):
MISMATCH = auto()
MISMATCH = _Mismatch.MISMATCHIn the function signature i used typing.Literal[_Mismatch.MISMATCH]. I replaced isinstance with identity checks and mypy seems to be happy with it.
What do you think?
There was a problem hiding this comment.
Currently py.typed is not included in the wheel.
There was a problem hiding this comment.
I googled a bit and I might have found out how. would be nice if you could have a look, though...
There was a problem hiding this comment.
No it still isn't included. It seems like the py.typed marker needs to be inside a package. But textparser is currently just a single module.
I don't know if mypy in cantools will be able to use these type annotations without the py.typed file.
I guess you have a mail in spam folder? @andlaus 
|
thanks to [at]zariiii9003! Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
|
indeed. thanks! |
it probably works, but I'm not completely sure. thanks to [at]zariiii9003 for pointing this out. Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
4 participants
For some of these I am not 100% sure if they are correct.
A lot of the code churn of this patch stems from the fact that some classes had to be reordered because python bails out if annotations use classes that are only defined later in the file.
This patch features a few minor functional changes which were neccessiated by type annotations:
setup.pyParser.grammar()methods always returnGrammarobjects instead of aPatternobject which gets implicitly converted toGrammarinParser.parse(). (IMO this makes the whole affair much less confusing and user code only needs to returnGrammar(x)instead ofx.)_Tokenclass is converted fromnamedtupleto a dataclass because named tuples do not play nicely with type annotations and the performance is basically identical._Stringclass now usesPatternas its base class. (It already had the same API asPattern, but not deriving_Stringfrom Pattern would necessiate_Stringto be handled separately anyway.)